refactor: Remove exceptions from RooHandler constructor #7302
Conversation
- RooHandler constructor no longer throws RooAuthenticationError - Removed RooAuthenticationError class entirely - Authentication errors now handled by provider-proxy server (401 responses) - Removed pre-flight authentication check from ClineProvider.createTask - Updated webviewMessageHandler to handle generic errors instead of RooAuthenticationError - Client gracefully handles server error responses This change ensures provider constructors never throw exceptions, delegating all error handling to the server level for better separation of concerns.
dosubot
bot
added
the
size:M
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for your contribution! I've reviewed the changes and have some suggestions for improvement. The refactor successfully removes exceptions from the constructor as intended, following the pattern established by other providers. The main concern is ensuring all mentioned changes are included and that error handling provides appropriate user feedback.
| providerName: "Roo Code Cloud", | ||
| baseURL: process.env.ROO_CODE_PROVIDER_URL ?? "https://api.roocode.com/proxy/v1", | ||
| apiKey: sessionToken, | ||
| apiKey: sessionToken || "unauthenticated", // Use a placeholder if no token |
There was a problem hiding this comment.
Using the string "unauthenticated" as a placeholder API key could be misleading. Consider using an empty string or a more descriptive placeholder like "NO_AUTH_TOKEN" to make the intent clearer:
| }) | ||
| } catch (error) { | ||
| // For all errors, reset the UI and show error | ||
| await provider.postMessageToWebview({ |
There was a problem hiding this comment.
The error handling here catches all errors generically but still sends a "newChat" message on error, which might not be the intended behavior. Also, the error message shown to the user doesn't distinguish between authentication errors and other types of errors. Consider:
- Should we really send "newChat" on error?
- Could we provide more specific error messages based on the error type?
| if (!sessionToken) { | ||
| throw new Error(t("common:errors.roo.authenticationRequired")) | ||
| if (CloudService.hasInstance()) { | ||
| sessionToken = CloudService.instance.authService?.getSessionToken() || "" |
There was a problem hiding this comment.
Consider adding debug logging when falling back to an empty token (without exposing the actual token value). This could help with debugging authentication issues.
The RooHandler constructor no longer throws exceptions when authentication is unavailable. Updated tests to expect the constructor to succeed gracefully in all cases, delegating authentication error handling to the server level.
github-project-automation
bot
moved this from PR [Needs Review]
to Done
in Roo Code Roadmap
Summary
This PR refactors the Roo provider to ensure that constructors never throw exceptions. Instead, authentication errors are now handled at the server level through HTTP status codes (e.g., 401 for unauthenticated requests).
Changes
ClineProvider.createTask()for the Roo provider has been removedRooAuthenticationErrorRationale
Based on feedback from PR #7298, provider constructors should never throw exceptions. This ensures:
Testing
Related
Important
Refactor
RooHandlerto avoid exceptions in constructor, handle authentication errors server-side, and update tests accordingly.RooHandlerconstructor no longer throws exceptions; uses empty token if session token unavailable.RooAuthenticationErrorclass.webviewMessageHandlerto use generic error handling.roo.spec.tsto ensureRooHandlerconstructor does not throw exceptions.This description was created by
for bbf9c13. You can customize this summary. It will automatically update as commits are pushed.